Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[core] Replace a useCallback by useRef in useEventCallback #39078

Merged
merged 1 commit into from
Oct 22, 2023
Merged

[core] Replace a useCallback by useRef in useEventCallback #39078

merged 1 commit into from
Oct 22, 2023

Conversation

romgrk
Copy link
Contributor

@romgrk romgrk commented Sep 20, 2023

Minor touchup, useRef is leaner than useCallback.

@romgrk romgrk added performance package: utils Specific to the @mui/utils package labels Sep 20, 2023
@mui-bot
Copy link

mui-bot commented Sep 20, 2023 •
edited
Loading

Netlify deploy preview

https://deploy-preview-39078--material-ui.netlify.app/

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against 5a2490c

@oliviertassinari oliviertassinari changed the title [perf] Replace a useCallback by useRef [core] Replace a useCallback by useRef Sep 20, 2023
@oliviertassinari oliviertassinari changed the title [core] Replace a useCallback by useRef [core] Replace a useCallback by useRef in useEventCallback Sep 20, 2023
@zannager zannager requested a review from mnajdova September 21, 2023 11:45
mnajdova
mnajdova approved these changes Oct 20, 2023
View reviewed changes
Copy link
Member

@mnajdova mnajdova left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM cc @michaldudak

@romgrk romgrk merged commit fdfaf64 into mui:master Oct 22, 2023
8 checks passed
@romgrk romgrk deleted the perf-use-event-callback branch October 22, 2023 17:40
@oliviertassinari
Copy link
Member

This makes me think of this thread: mui/toolpad#2671 (comment) I had with @Janpot. We initially copied https://github.com/reactjs/rfcs/blob/useevent/text/0000-useevent.md#internal-implementation

@romgrk
Copy link
Contributor Author

romgrk commented Oct 25, 2023

Tbh I feel our useEventCallback is a bit weird. In the React RFC you linked, the ref is initialized as null under the rationale that they want it to throw if it's called during render, for reasons that make sense from a correctness/DX POV (even if a bit pedantic, but I get they need it in react core). But in our implementation we both initialize the ref with the callback directly AND we update it in the useLayoutEffect. So we both don't get the benefit of throw-during-render, and we pay the cost of adding an effect.

@Janpot
Copy link
Member

Janpot commented Oct 30, 2023

For reference, this was our implementation: https://github.com/mui/mui-toolpad/blob/6cd4f8f289472d75811a39cc6ef567dc2024e85f/packages/toolpad-app/src/utils/useEvent.ts

@albarv340 albarv340 mentioned this pull request Nov 28, 2024
Open
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mnajdova mnajdova mnajdova approved these changes

Assignees
No one assigned
Labels
package: utils Specific to the @mui/utils package performance
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@romgrk @mui-bot @oliviertassinari @Janpot @mnajdova